Added named regions for standalone relations #931
Conversation
| @@ -85,14 +85,17 @@ const PROF_MSG_BUF_SIZE: usize = 10_000; | |||
| pub type Response<X> = Result<X, String>; | |||
|
|
|||
| /// Unique identifier of a DDlog relation. | |||
| // TODO: Newtype this for type-safety | |||
There was a problem hiding this comment.
These types are visible all the way from the C API. I therefore wanted them to look like numbers. Would newtyping break that or does struct RelId(usize) look like a usize from C?
There was a problem hiding this comment.
#[repr(transparent)] struct wrappers will have the exact same abi is the inner type, so
#[repr(transparent)]
struct RelationId(usize);will have the same abi as a usize while still allowing type safety within rust
|
|
||
| collection | ||
| }); | ||
| scope.clone().region_named(relation.name(), |region| { |
There was a problem hiding this comment.
So we're using regions to make relation/rule metadata visible to external tools (e.g., debuggers). I didn't realize this would be the case, but I guess it makes sense. For operators, I was under the impression that they all now have _named versions, so we could use those instead, but it looks like there's only a small number of _named operators (threshold_named, reduce_named, and a few more).
Do you know if regions have any runtime cost?
There was a problem hiding this comment.
I'm doing both, regions visually organize things while named operators give details on individual operations. Named regions should have a trivial to undetectable amount of overhead since they're just scoped() invocations that don't change the timestamp (and therefore don't incur the cost of syncing at changing timestamps)
|
We should talk about the long term plans for the compiler. |
Yes, I've been planning on this and the subsequent refactors I make should allow that task to be significantly easier if all goes well |
Kixiron
force-pushed
the
operator-names
branch
4 times, most recently
from
b4ad845
to
e3dc240
Compare
|
The issue as of now seems to be this piece of code and the associated pointer casts that are basically transmutes |
|
Well that was exceedingly painful and makes me ever more convinced that we need to switch to a |
It certainly looks that way.
The only reason we're not doing this is because depending on the kind of transform the next element in the list can be either |
|
I'm fairly confident that can be made nice to work with given nicely-segmented structs and all that jazz, at minimum an enum |
I think so. It will sacrifice a bit of compile-time type checking, which is still better than having all these hacks. |
|
I think it's fixed, but now some of the tests time out for some wonderful reason |
| is_top_level_scope, | ||
| ) | ||
| } else { | ||
| Self::streamless_xform_collection_ref( |
There was a problem hiding this comment.
This might be wrong. This assumes that only the first operator in a rule can be a stream xform and will break otherwise. In practice, this means that if a streaming group_by occurs after some other operators, e.g., streaming join, things will break. I will try to create a counterexample, but may not have the time until later today.
|
Duplication does suck, and this is what I was trying to avoid with unsafe conversions, but I guess this works. |
|
I'll address it in a follow up PR, it just requires some groundwork from this one |
|
I notice that tests run more slowly in this branch. Might be load variations on the CI machine, but I want to investigate. |
|
Opened timely-dataflow/#382 |
Recent DD patches include some bug fixes, including at least two bugs that affected our ongoing profiling work.
…w and dogsdogsdogs
This is part one of what I suspect will be a saga of similar changes. This pr adds a named region encompassing each standalone (non-recursive) relation. For further changes I'm reasonably sure I'll have to make much more sweeping changes to the dataflow rendering code, which will be painful but highly beneficial in the long term as it'll become significantly easier to reason about and modify, as well as being less prone to fun little lifetime sinkholes.